fix(homelab): make memory and ZFS alerts durable - #2008
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
PR Summary by QodoFix memory offset logic and make ZFS maintenance pod-aware for durable alerts
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9228a348a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const NAMESPACE = "prometheus"; | ||
| const TARGET = "daemonset/zfs-zpool-collector"; | ||
| const POOLS = ["zfspv-pool-nvme", "zfspv-pool-hdd"] as const; | ||
| const ZFS_COLLECTOR_LABEL = "app=zfs-zpool-collector"; |
There was a problem hiding this comment.
The new discovery query selects app=zfs-zpool-collector, but zfs-zpool.ts places that label only on the DaemonSet's metadata, not its podMetadata; Kubernetes does not propagate workload labels to pods, and the replaced code explicitly documented that these pods only have cdk8s-generated labels. Consequently the API returns no candidates and every weekly maintenance run fails before scrubbing any pool. Add the label to the pod template or select using a label the pods actually carry.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
| if (isReady !== true || pod.status?.phase !== "Running") { | ||
| continue; |
There was a problem hiding this comment.
When one node's collector is Pending, CrashLooping, or otherwise not Ready while another node's collector is healthy, this silently drops the unavailable node and returns a successful maintenance result for only the healthy node. That leaves the skipped node's pool unscrubbed and prevents Temporal's workflow-failure alerting from reporting the incomplete run; validate that every node represented by the collector candidates has a selected Ready pod and fail otherwise.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
Code Review by Qodo
1. PromQL string exceeds 120 chars
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| expect(alert.expr.value).toBe( | ||
| "((node_memory_MemTotal_bytes - node_memory_MemAvailable_bytes) - on(instance) group_left node_zfs_arc_size) - ((node_memory_MemTotal_bytes offset 24h - node_memory_MemAvailable_bytes offset 24h) - on(instance) group_left node_zfs_arc_size offset 24h) > 8589934592", | ||
| ); |
There was a problem hiding this comment.
1. Promql string exceeds 120 chars 📘 Rule violation ⚙ Maintainability
New PromQL expressions are embedded as single-line string literals that exceed the 120-character maximum, which will violate the repo's line-length compliance and hinder readability/maintenance.
Agent Prompt
## Issue description
The PR adds lines that exceed the 120-character maximum due to long single-line PromQL string literals.
## Issue Context
This repo enforces a 120-character max line length for non-generated source files.
## Fix Focus Areas
- packages/homelab/src/cdk8s/src/resources/monitoring/monitoring/rules/resource-monitoring.test.ts[112-114]
- packages/homelab/src/cdk8s/src/resources/monitoring/monitoring/rules/resource-monitoring.ts[93-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "zfs_zpool_last_scrub_completion_timestamp == 0 or (time() - zfs_zpool_last_scrub_completion_timestamp) > 777600", | ||
| ), |
There was a problem hiding this comment.
2. Scrub alert value zero 🐞 Bug ◔ Observability
ZfsScrubOverdue now uses ts == 0 or (time() - ts) > 777600, which (for never-scrubbed pools) yields a sample value of 0 due to or preferring the left-hand side. The alert still fires, but the notification value becomes less useful (it no longer reflects elapsed time for the never-scrubbed case).
Agent Prompt
## Issue description
The `ZfsScrubOverdue` PromQL expression uses `A or B` where `A` is `zfs_zpool_last_scrub_completion_timestamp == 0`. For pools that have never scrubbed, this causes Prometheus to keep the left-hand sample value (`0`) instead of the elapsed-time value, reducing the usefulness of `$value` in alert notifications.
## Issue Context
The collector script explicitly emits `zfs_zpool_last_scrub_completion_timestamp` as `0` when no completed scrub is recorded.
## Fix Focus Areas
- packages/homelab/src/cdk8s/src/resources/monitoring/monitoring/rules/zfs-maintenance.ts[52-66]
- packages/homelab/src/cdk8s/src/resources/monitoring/monitoring/rules/zfs-maintenance.test.ts[18-20]
## Suggested fix
Prefer a single elapsed-time condition that already covers `ts == 0`, e.g.:
- `time() - zfs_zpool_last_scrub_completion_timestamp > 777600`
This keeps the alert firing for never-scrubbed pools (since `time() - 0` is large) while preserving an elapsed-time value for triage. Update the unit test expectation accordingly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
Verification
No live deployment, scrub, or PagerDuty mutation was performed.